multiple SSPs/BEs support for SOF--Kernel Part - #4
Conversation
plbossart
left a comment
There was a problem hiding this comment.
I don't really understand what this patchset fixes compared to the previous version, this is still somewhat messy in terms of link declaration, setup and handling of multiple outputs.
There was a problem hiding this comment.
it original from Liam, And it is the maximum of cpu_dai caps, it can still be reduced by the FE/PCM caps:
hw->channels_min = max(chan_min, cpu_stream->channels_min);
hw->channels_max = min(chan_max, cpu_stream->channels_max);
so I think leaving them as 16 has no harm.
There was a problem hiding this comment.
8 chan is correct, my original value is wrong.
There was a problem hiding this comment.
OK, let me change them to 8 in v3.
There was a problem hiding this comment.
Can you look at Rakesh's patches, I think we could update this with Analog, Alt-Analog and Digital?
There was a problem hiding this comment.
OK, I will go and get that for HDMI enabling, I believe we may need to refine them during enabling each BEs, as I talked previously, it's not one shot fix, it is only framework implementation here.
There was a problem hiding this comment.
keyon, can you check the HDA/HDMI naming should be consistent per platform (but it may change between different platforms).
There was a problem hiding this comment.
confirmed they are same with cAVS ones defined in skl-pcm.c
There was a problem hiding this comment.
I meant use something this instead, see alsa-devel [PATCH v3 11/13] ASoC: hdac_hda: add asoc extension for legacy HDA codec drivers
+static struct snd_soc_dai_driver hdac_hda_dais[] = {
+{
- .id = HDAC_ANALOG_DAI_ID,
- .name = "Analog Codec DAI",
- .ops = &hdac_hda_dai_ops,
- .playback = {
-
.stream_name = "Analog Codec Playback", -
.channels_min = 1, -
.channels_max = 16, -
.rates = SNDRV_PCM_RATE_8000_192000, -
.formats = STUB_FORMATS, -
.sig_bits = 24, - },
- .capture = {
-
.stream_name = "Analog Codec Capture", -
.channels_min = 1, -
.channels_max = 16, -
.rates = SNDRV_PCM_RATE_8000_192000, -
.formats = STUB_FORMATS, -
.sig_bits = 24, - },
+},
+{ - .id = HDAC_DIGITAL_DAI_ID,
- .name = "Digital Codec DAI",
- .ops = &hdac_hda_dai_ops,
- .playback = {
-
.stream_name = "Digital Codec Playback", -
.channels_min = 1, -
.channels_max = 16, -
.rates = SNDRV_PCM_RATE_8000_192000, -
.formats = STUB_FORMATS, -
.sig_bits = 24, - },
- .capture = {
-
.stream_name = "Digital Codec Capture", -
.channels_min = 1, -
.channels_max = 16, -
.rates = SNDRV_PCM_RATE_8000_192000, -
.formats = STUB_FORMATS, -
.sig_bits = 24, - },
+},
+{ - .id = HDAC_ALT_ANALOG_DAI_ID,
- .name = "Alt Analog Codec DAI",
- .ops = &hdac_hda_dai_ops,
- .playback = {
-
.stream_name = "Alt Analog Codec Playback", -
.channels_min = 1, -
.channels_max = 16, -
.rates = SNDRV_PCM_RATE_8000_192000, -
.formats = STUB_FORMATS, -
.sig_bits = 24, - },
- .capture = {
-
.stream_name = "Alt Analog Codec Capture", -
.channels_min = 1, -
.channels_max = 16, -
.rates = SNDRV_PCM_RATE_8000_192000, -
.formats = STUB_FORMATS, -
.sig_bits = 24, - },
+}
There was a problem hiding this comment.
so we need 3 for HDA legacy codecs? And should this be same for all platforms?
There was a problem hiding this comment.
I still don't get how we handle multiple nocodec outputs with this solution?
There was a problem hiding this comment.
multiple nocodec cpu_dais are created during probing, each dai driver defined in platform driver(e.g. apl.c) will be used and corresponding nocodec dai_link will be created in sof_nocodec_setup().
There was a problem hiding this comment.
Is there a way to use common tables when APL/CNL/ICL are identical?
There was a problem hiding this comment.
it is possible, but separating them is more flexible at the moment in my opinion, if they are totally same after all our features are implemented and they do have no difference, we can merge them then.
There was a problem hiding this comment.
SSP rates on all the platforms are the same atm, so safe to merge.
There was a problem hiding this comment.
Hi Liam, sorry can't get your point here, so do all of us aligned on using common tables for APL/CNL/ICL?
There was a problem hiding this comment.
how about putting it in hda-pcm.c?
There was a problem hiding this comment.
Pretty sure SKL/KBL don't have 6 SSPs
There was a problem hiding this comment.
by the way, do you know how much we have for that? I get information from cAVS 1.5 HAS that I2SPC is 3, but the document claims it support all BXT/GLK/SPT platforms.
There was a problem hiding this comment.
per my understanding, for SKL+, cAVS driver use the common table defined in skl-pcm.c, that is the same table we defined in apl.c/cnl.c/skl.c, so here for SKL/KBL, let's keep it, which is same with cAVS one?
There was a problem hiding this comment.
I don't understand why we set formats that are not valid except for SSP, and how we deal with DMIC or HDAudio with this work.
Specifically with DMIC we know it's capture only so shouldn't we look at the information in ops->drv[i] to avoid changing things that make limited sense?
There was a problem hiding this comment.
here it is for nocodec only, but maybe you are right, if somebody defines to use nocodec DMIC in tplg file, then...
again, this BE.dai_fmt for nocodec actually don't take any effect, maybe I should remove this assignment here to make you feel more relax. :)
There was a problem hiding this comment.
is this still needed to have a nocodec-dai?
There was a problem hiding this comment.
good point, let me fix it, thanks.
|
update to v2 with addressing some comments. |
There was a problem hiding this comment.
This file can be completely removed now and this function should be in sof/utils.c
I would also change this function to sof_platform_add_all_bes()
There was a problem hiding this comment.
how about the probe()/remove(), the platform driver, and the card sound card register invoking?
There was a problem hiding this comment.
They are fine, they should register the card and get around any deferred probe binding.
There was a problem hiding this comment.
We are no longer "sof-nocodec" as this code can be used for platforms that support add-in audio cards/devices. We should probably pass this in.
There was a problem hiding this comment.
Liam's comment was not addressed?
There was a problem hiding this comment.
I had a call with Liam about this, and aligned to keep this no-codec driver and keep it simple. The reason for it is that we can't leverage deffer probe without using platform probing.
There was a problem hiding this comment.
rename nocodec_ -> sof_platform_
There was a problem hiding this comment.
same, Liam's comment isn't addressed.
There was a problem hiding this comment.
What happens if this fails ? Need to handle errors
There was a problem hiding this comment.
I'll merge this into my upstream patch.
Register all DAI from platform driver Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
lgirdwood
left a comment
There was a problem hiding this comment.
Just minor things to fix for me.
There was a problem hiding this comment.
It's probably worth commenting here that some products who use this DAI array only physically have a subset of the DAIs, but no harm is done here by adding the whole set.
There was a problem hiding this comment.
CHT has 6 SSPs, best to add them and include a comment.
There was a problem hiding this comment.
are you sure CHT has 6 SSPs? I see from your previous patch series that it is set and commented as 3/
There was a problem hiding this comment.
and 3 is same with sst-mfld-platform-pcm.c also.
/* BE CPU Dais */
{
.name = "ssp0-port",
.ops = &sst_be_dai_ops,
.playback = {
.stream_name = "ssp0 Tx",
.channels_min = SST_STEREO,
.channels_max = SST_STEREO,
.rates = SNDRV_PCM_RATE_48000,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
},
.capture = {
.stream_name = "ssp0 Rx",
.channels_min = SST_STEREO,
.channels_max = SST_STEREO,
.rates = SNDRV_PCM_RATE_48000,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
},
},
{
.name = "ssp1-port",
.ops = &sst_be_dai_ops,
.playback = {
.stream_name = "ssp1 Tx",
.channels_min = SST_STEREO,
.channels_max = SST_STEREO,
.rates = SNDRV_PCM_RATE_8000|SNDRV_PCM_RATE_16000|SNDRV_PCM_RATE_48000,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
},
.capture = {
.stream_name = "ssp1 Rx",
.channels_min = SST_STEREO,
.channels_max = SST_STEREO,
.rates = SNDRV_PCM_RATE_8000|SNDRV_PCM_RATE_16000|SNDRV_PCM_RATE_48000,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
},
},
{
.name = "ssp2-port",
.ops = &sst_be_dai_ops,
.playback = {
.stream_name = "ssp2 Tx",
.channels_min = SST_STEREO,
.channels_max = SST_STEREO,
.rates = SNDRV_PCM_RATE_48000,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
},
.capture = {
.stream_name = "ssp2 Rx",
.channels_min = SST_STEREO,
.channels_max = SST_STEREO,
.rates = SNDRV_PCM_RATE_48000,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
},
},
};
There was a problem hiding this comment.
They are fine, they should register the card and get around any deferred probe binding.
|
You may need to refine the the code here to make dai link can be found for the codec machine drivers. Line 1001 in cb05347 This will cause some machine driver dai could not found. It may fix thesofproject/soft#9 |
|
xiuli, the issue you mentioned should be fixed in tplg file, we should set .id value for DAI in test codec tplg. |
|
Liam/Pierre, just updated, please help review. |
plbossart
left a comment
There was a problem hiding this comment.
Almost looks good, just a couple of minors comments to look into. Thanks Keyon.
There was a problem hiding this comment.
why is this needed? Can we add later when it's actually doing something?
There was a problem hiding this comment.
are you meaning this hda-dai.c file? this is suggested by Liam to add, and it addressing your comment about common dai driver table for SKL/APL/CNL also.
There was a problem hiding this comment.
I believe the max number of channels should be 16 for HDAudio (same for all digital parts below)
There was a problem hiding this comment.
please feel free to change it, as I am not able to test them yet, so I can't give any comment about it.
There was a problem hiding this comment.
do we support FLOAT on BDW? (same comments for all machines btw, I don't think we can deal with FLOAT on a DAI.
There was a problem hiding this comment.
This is also initiated by Liam, Liam, do you have comment on it?
There was a problem hiding this comment.
Liam's comment was not addressed?
There was a problem hiding this comment.
same, Liam's comment isn't addressed.
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
This patch create nocodec BE DAI links based on platform dai drivers, with it, we can support multiple SSP/BEs for nocodec mode. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
1. Don't override cpu_dai_name, we will reuse those defined in machine driver. 2. We should keep dpcm_playback and dpcm_capture defined for BEs, not both of them are supported by BEs sometimes. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
…dynamic We should use .dynamic for all FE dai_links, so change probe and trace buffer ones here to align to that. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
|
Hi Pierre/Liam, new version updated, I think it address all comments from you, please help review. |
plbossart
left a comment
There was a problem hiding this comment.
There is one isolated 16->8 ch issue that needs to be fixed in a follow-up patch. let's merge this.
| .name = "SSP2 Pin", | ||
| .playback = SOF_DAI_STREAM("ssp2 Tx", 1, 8, | ||
| SNDRV_PCM_RATE_8000_192000, SKL_FORMATS), | ||
| .capture = SOF_DAI_STREAM("ssp2 Rx", 1, 16, |
There was a problem hiding this comment.
oops, let me send incremental fix soon.
We need SOFT part merged together, that is stream_name rename for nocodec topology to make nocodec topology works.